Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KNOX-3065: SSE support for Knox #947

Merged
merged 1 commit into from
Nov 11, 2024
Merged

KNOX-3065: SSE support for Knox #947

merged 1 commit into from
Nov 11, 2024

Conversation

hanicz
Copy link
Contributor

@hanicz hanicz commented Oct 22, 2024

What changes were proposed in this pull request?

SSE support for Knox.

Currently Knox is not working with SSE. In case of SSE the response header contains Content-Type = text/event-stream, in which case the server will send messages terminated by \n\n. Knox should send these messages to the client as they arrive. Currently Knox collects them all, and once the server closes the connection sends all of them concatenated to each other to the client.

Requirement:

Full support in Knox for Server Sent Events.
Definition of done:

Clients can connect to SSE endpoints in the backend via Knox.
Server Sent Events (https://html.spec.whatwg.org/multipage/server-sent-events.html#server-sent-events) are forwarded by Knox from the server to the client as soon as they are received.

How was this patch tested?

Unit tests.
Locally tested with a custom SSE service.
Any SSE service that is the follows the specs is supported in Knox.
Setup:
Asynchronous behaviour and therefore SSE support is not enabled by default. Modify the below config to ‘true’ in /conf/gateway-site.xml file.

<property>
   <name>gateway.servlet.async.supported</name>
   <value>true</value>
   <description>Enable/Disable async support.</description>
</property>

Change the existing topology by adding an SSE service to /conf/{topology}.xml

<service>
   <role>SSERVICE</role>
   <url>http://localhost:7435</url>
</service>

Add the SSE service to the <KNOX_HOME>/data/services folder.

/data/services/{myservice}/{version}/rewrite.xml

<rules>
   <rule dir="IN" name="SSERVICE/sservice/inbound" pattern="*://*:*/**/sservice/*">
       <rewrite template="{$serviceUrl[SSERVICE]}/sse"/>
   </rule>
</rules>

/data/services/{myservice}/{version}/service.xml (Make sure to add the SSEDispatch to the service)

<service role="SSERVICE" name="sservice" version="0.1">
   <routes>
       <route path="/sservice/**"></route>
   </routes>
   <dispatch classname="org.apache.knox.gateway.sse.SSEDispatch"/>
</service>

curl command to test the configured SSE service:
curl -k -i -u admin:admin-password https://localhost:8443/gateway/sandbox/sservice/sse

GET, POST, PUT, PATCH methods were tested.

@smolnar82
Copy link
Contributor

Hi @hanicz !

Could you please elaborate on the tests you executed with a custom SSE service? Future readers will benefit from it if they want to configure such an environment.
Thanks!

@moresandeep
Copy link
Contributor

Hi @hanicz !

Could you please elaborate on the tests you executed with a custom SSE service? Future readers will benefit from it if they want to configure such an environment. Thanks!

Agreed, thanks for chiming in @smolnar82!

@hanicz
Copy link
Contributor Author

hanicz commented Oct 30, 2024

Fixed the JLS issue in a new commit and added my local test setup to my initial comment.

@hanicz
Copy link
Contributor Author

hanicz commented Oct 30, 2024

The test fail is unrelated to my change. The cause seems to be this PR

@smolnar82
Copy link
Contributor

Hi @hanicz !

The unit test failure is now fixed by your other PR (#948 ) so you can rebase this one on top of that.

@hanicz hanicz force-pushed the KNOX-3065 branch 2 times, most recently from a7e881a to 0f1457f Compare November 5, 2024 09:35
Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly some nit-picking about method names. Otherwise, it looks like a good addition.

@hanicz hanicz force-pushed the KNOX-3065 branch 3 times, most recently from 0d1cb55 to 0106780 Compare November 8, 2024 09:52
pzampino
pzampino approved these changes Nov 8, 2024
@pzampino pzampino self-requested a review November 8, 2024 20:45
Copy link
Contributor

@pzampino pzampino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more log message concern.

Copy link
Contributor

@moresandeep moresandeep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. thank you @hanicz !

@pzampino pzampino merged commit 58dd02b into apache:master Nov 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants